feat(stamphog): make size_gate.max_lines delegable per folder - #91348
Conversation
|
😎 Merged successfully - details. |
🤖 CI report
|
Prompt To Fix All With AI### Issue 1
products/stamphog/packages/pr-approval-agent/policy.py:657-668
**Inherited file budgets multiply**
When a folder grants only `max_lines`, `resolve` creates a separate scope with a fresh copy of the global or ancestor `max_files` allowance. Because `_check_size` checks each scope independently, 20 substantive desktop files and 20 global files pass the nominal 20-file gate even though no folder granted a higher file limit.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "Raise desktop line ceiling to 1000 via f..." | Re-trigger Greptile |
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
HostHog preview —
|
charlesvien
added
the
desktop-skip-backend-check
PostHog
A folder that granted one size_gate key was given a fresh copy of the other key's global ceiling, which created a second budget nobody delegated: a lines-only grant let a PR touch 30 folder files plus 30 global files, and a files-only grant let it add 800 folder lines plus 800 global lines. Each ceiling now resolves on its own chain walk. A file counts against the nearest folder granting that ceiling and against the global pool when the chain grants it nowhere, so a one-key grant leaves its files in the pool they were already in for the other key. Generated-By: PostHog Desktop Task-Id: dd27894e-233a-469b-ada8-c80d13d67405
Generated-By: PostHog Desktop Task-Id: dd27894e-233a-469b-ada8-c80d13d67405
The size-gate helpers returned same-typed pairs, which the tuple-return-prefer-dataclass rule blocks: callers can swap the two scope tuples, or the two counts, without a type error. _size_budgets now returns the EffectivePolicy the scopes already live on, and the per-scope file filter returns the file list so substantive_size stays the one place that unpacks a count pair. Generated-By: PostHog Desktop Task-Id: dd27894e-233a-469b-ada8-c80d13d67405
charlesvien
added
the
stamphog
PostHog
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
webjunkie
left a comment
webjunkie
left a comment
There was a problem hiding this comment.
Note
Agent review, actively steered by @webjunkie. Every claim names its evidence.
Approving. The delegation mechanics are correct. One adjustment to consider, not a blocker.
Verified
- Per-key chain resolution works as documented, and a mixed grant with one over-ceiling key invalidates the whole file (
test_policy.py,one-key-over-ceiling). - The evidence JSON
scopesshape change breaks no consumer. Nothing inproducts/stamphog/backend,products/stamphog/mcp, or the generated frontend types reads it. - The desktop grant covers what its prose claims.
packages/core,packages/ui, andapps/all sit underproducts/desktop/.
Consider restoring a whole-PR total
_check_size no longer compares anything to the whole-PR size. The old if lines > max_lines is gone, and lines, files = substantive_size(self.pr.files) now only decorates the refusal strings. Every check is per-scope, and scope count is unbounded.
| PR shape | this PR allows | with a roof |
|---|---|---|
| global only | 800 | 800 |
| desktop only | 1000 | 1000 |
| desktop plus global | 1800 | 1000 |
| desktop plus 4 more granting folders | 4800 | 1000 |
That conflicts with the rationale this PR keeps in pr-approval-agent/README.md: "past ~800 the merged-unchanged rate collapses". The threshold was derived from PR size, not per-folder size.
max(s.ceiling for s in budgets.line_scopes) compared to the whole-PR lines closes it, and the same expression over file_scopes closes the pre-existing files case. It needs no new key, since every grant is already validated at or under the contract ceiling. With no grant in play it equals the global default, so it is the deleted check with one variable swapped. It leaks no headroom, because the per-scope budgets still cap the global pool at 800.
Refutable. If per-scope budgets are meant to sum without bound, both READMEs should say so, and the "~800" rationale needs rewording because it no longer describes any check.
A stacked PR with this change follows, to accept or drop.
|
/trunk merge |
2 participants
Problem
A folder
AGENT_APPROVALS.mdcan raise stamphog's file ceiling for its own files, but not the 800 line ceiling. Teams with larger PRs on average get no per-folder relief.Changes
size_gate.max_linesis now delegable, ceiling 1000, alone or next tomax_files.products/desktop/AGENT_APPROVALS.mdgrantsmax_lines: 1000, so desktop PRs up to 1000 substantive lines stay eligible for auto-review.ScopeBudgetand the evidence JSON gainmax_lines, bothpolicy.ymlfiles declare the key, docs updated.How did you test this code?
test_policy.pycases cover lines-only grants, child and parent key inheritance, over-ceilingmax_linesand per-scope line overruns.